-
Notifications
You must be signed in to change notification settings - Fork 257
C# future simple codegen #1357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C# future simple codegen #1357
Conversation
b14b14b to
c3fce61
Compare
|
Thanks for doing this, @yowl! I'm planning to review it by the end of the week. |
dicej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! Looks like a good start; please see my comments inline.
Combine FutureReader and FutureWriter to AsyncSupport. Start the process of adding futures per type
|
@dicej Hi, I think I've covered the points from the review now, was a bit of a change for the generic refactoring, sorry about that churn. |
dicej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for continuing to iterate on this, @yowl, and sorry for the delayed review.
Looks good overall; just a few more comments inline.
crates/csharp/src/AsyncSupport.cs
Outdated
|
|
||
| public static WaitableSet WaitableSetNew() | ||
| {{ | ||
| var waitable = Interop.WaitableSetNew(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: consider naming this waitableSet or handle. The name waitable is a bit confusing since that term means something else in the component model.
crates/csharp/src/AsyncSupport.cs
Outdated
| void Dispose(bool _disposing) | ||
| { | ||
| // Free unmanaged resources if any. | ||
| vTable.DropReader(Handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should only call this if Handle != 0
crates/csharp/src/AsyncSupport.cs
Outdated
| void Dispose(bool _disposing) | ||
| { | ||
| // Free unmanaged resources if any. | ||
| vTable.DropReader(Handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should only call this if Handle != 0
crates/csharp/src/AsyncSupport.cs
Outdated
| void Dispose(bool _disposing) | ||
| { | ||
| // Free unmanaged resources if any. | ||
| VTable.DropWriter(Handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should only call this if Handle != 0.
crates/csharp/src/function.rs
Outdated
| .collect::<Vec<_>>(); | ||
| let ty = self.interface_gen.type_name_with_qualifier(ty, true); | ||
| //let ty = self.gen.type_name(ty); | ||
| //let ty = self.r#gentype_name(ty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| //let ty = self.r#gentype_name(ty); | |
| //let ty = self.interface_gen.type_name(ty); |
Or can we just get rid of this line?
|
I think that's all the latest comments addressed. Thanks for the help! |
dicej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just one more item and this should be good to merge.
|
Thanks you! |
This PR adds enough code gen to support the simple-future wit runtime test. As for the async PR, this is pretty much the minimum PR in terms of future support. I've not tackled the typed canonical methods except to add a "void" implementation which is hard coded as the one to use.
Have followed the c test cases rather than the rust ones.
Also changed Export and Import in namespaces to be uppercase and moved resources and other methods to the appropriate import or export class. Some types are still produced from the import side, and have introduced a concept of a bidirectional type (enum, flags) that sit above the import/export split.
The current codegen produced is at https://github.com/yowl/wit-bindgen-simple-future